Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: sync state on undo when editor is unfocused #5737

Merged
merged 11 commits into from
Oct 16, 2024

Conversation

WindRunnerMax
Copy link
Contributor

@WindRunnerMax WindRunnerMax commented Oct 9, 2024

Description
Undo when the editor is not in focus will change editor content, but won't update editor state.

In my tests, this issue only appears on Chrome/Chromium. It doesn't occur on Firefox, Safari, or Edge. However, the #5544 report indicates that it also happens on Edge and Safari, so I didn't limit the patch to Chrome.

Issue
Fixes: #5544

Example

After entering some content, click elsewhere to make the editor lose focus. Then use Ctrl/Cmd+Z to undo. When you start typing again, you'll notice the previously undone content reappears.

After making adjustments.

Context

I tried to get to the root of the problem. If you completely preventDefault the beforeinput event, nothing will be placed in the browser's undo stack, so input events like historyUndo won't be triggered.

As mentioned in the comments, if you completely block the event, we won't be able to handle undo events in beforeinput.

// COMPAT: Since we prevent the default behavior on
// `beforeinput` events, the browser doesn't think there's ever
// any history stack to undo or redo, so we have to manage these
// hotkeys ourselves. (2019/11/06)

However, because there's a conditional preventDefault in the beforeinput event, some content will still enter the browser's undo stack. For example, typing 123 won't cause the issue, but typing abc will trigger the undo problem when losing focus.

if (!native) {
event.preventDefault()
}

I came up with four solutions to address this issue.

  1. Completely prevent the default behavior of the beforeinput event. However, this could be a major disruptive change and might not be suitable for solving this problem.
  2. Completely prevent the browser's undo action when losing focus. After researching the issue, I found that to prevent it, we need to listen to the document's onKeyDown event (since the editor loses focus and won't trigger its own onKeyDown event). This might require "excessive" permissions.
  3. Sync the editor's state when undoing after losing focus. This solution requires us to actively trigger the editor's undo/redo methods when this action occurs to sync the editor's built-in state.
  4. Clear the undo stack for the DOM nodes. But as far as I know, there's no way to do that right now. Maybe a future undo manager(undo manager w3c/editing#209) or API could make it possible.

In the end, I chose option 3. I found that triggering this behavior only fires the oninput event of that node, while the onbeforeinput and onkeydown events don't get triggered. It's quite fascinating.

Also, I tried to prevent the undo action in this input event, but it didn't work. event.preventDefault didn't do the trick here. So, I decided to check whether we should be in edit mode to determine if we should manually call the editor's undo/redo methods. Interestingly, when this action is triggered, we can determine the state by checking the focus.

In this case, I ran a lot of tests to avoid any unexpected undo/redo actions.

  1. Triggering an input event without focus is actually a patch for special behavior. When there's focus, this logic won't apply. Also, this event is specific to the DOM node, not triggered globally, so it's related to the state of the node itself.
  2. The beforeinput event will call preventDefault for inputTypes of historyUndo and historyRedo, so the input event won't proceed here.
  3. The onkeydown event for undo/redo will also call preventDefault, so the input event won't be triggered.
  4. I mocked a flag indicating that the beforeinput event is not supported. In this case, all input will be prevented, nothing will enter the browser's undo stack, and the input event won't be triggered.

Checks

  • The new code matches the existing patterns and styles.
  • The tests pass with yarn test.
  • The linter passes with yarn lint. (Fix errors with yarn fix.)
  • The relevant examples still work. (Run examples with yarn start.)
  • You've added a changeset if changing functionality. (Add one with yarn changeset add.)

Copy link

changeset-bot bot commented Oct 9, 2024

🦋 Changeset detected

Latest commit: e00d08e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
slate-react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Collaborator

@dylans dylans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable, it is triggering a failing test/example so fixing that would be cool.

@WindRunnerMax
Copy link
Contributor Author

@dylans Sorry, Actions didn't give me any feedback last time, so I thought all the tests were successful.

But now it seems like there's an issue with the test:integration environment.

Comparing the old test results with the new ones, I found that the previous test results for Chromium were successful.

Old: https://github.com/ianstormtaylor/slate/actions/runs/11258958959
New: https://github.com/ianstormtaylor/slate/actions/runs/11356967135

Regarding the issue with the test environment failure, maybe I can handle it :)

@WindRunnerMax
Copy link
Contributor Author

I merged the fix #5742 for the test:integration environment into my own test branch , and the tests ran smoothly.

https://github.com/WindRunnerMax/slate/actions/runs/11364143647

* fix: fix firefox test integration env

* chore: test ubuntu apt source

* chore: IMMUTABLE_INSTALLS ?

* fix: ubuntu version
@dylans
Copy link
Collaborator

dylans commented Oct 16, 2024

@WindRunnerMax it looks like we're on a very old version of Node (12) which is then being kicked to the oldest available (16) which is likely too old. If you have time to upgrade the ci config for Slate to use Node 20, awesome, otherwise I'll get to that this week.

@WindRunnerMax
Copy link
Contributor Author

@dylans Sure, I have time to finish this task this week.

Also, I rebase the current main branch, triggered the CI tests, and the results were good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Undo when the editor is not in focus causes wrong data
3 participants